Add analysis mode for Seninel-1 SLC#86
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
TonioF
left a comment
TonioF
left a comment
There was a problem hiding this comment.
See my comments. To be honest, I find this pr so massive that I could only examine that it looks good, but I could not really verify what it does.
I also tried to run the notebook, I ended up with RasterioIOError: HTTP response code: 403 .
| > currently experimental and undergoing validation. | ||
| > currently experimental and undergoing validation. Some conversion parameters | ||
| > are missing in the new EOPF product, which are currently estimated. Newer EOPF | ||
| > product verison will include these parameters. |
There was a problem hiding this comment.
| > product verison will include these parameters. | |
| > product version will include these parameters. |
| (`azimuth_time`, `slant_range_time`) and organized in bursts and swaths. To transform | ||
| this data into an **analysis-ready dataset**, the following processing steps are applied: | ||
|
|
||
| 1. **Radiometric Calibration:** For each burst complex SLC measurements for are |
There was a problem hiding this comment.
| 1. **Radiometric Calibration:** For each burst complex SLC measurements for are | |
| 1. **Radiometric Calibration:** For each burst complex SLC measurements are |
| If not specified, [EPSG:4326](https://epsg.io/4326) is used. | ||
| - `resolution`: Target resolution for all spatial variables expressed in the units | ||
| of the specified `crs`. If not specified, the resolution is derived (in degrees) | ||
| from the CopDEM (30 m). |
There was a problem hiding this comment.
Is that the same DEM as described in the next parameter?
There was a problem hiding this comment.
yes, I added this information.
| url = ( | ||
| "https://data.eodc.eu/collections/EOPF_ZARR/products/cpm_v270/" | ||
| "S01SIWSLC/2026/05/31/S1D_IW_SLC__1SDV_20260531T171503_" | ||
| "20260531T171530_003031_00539E_C573.zarr" | ||
| ) |
There was a problem hiding this comment.
It's unfortunate to have a url to an external resource in a test. If it is no option to host this (or a subset) on GitHub, add an annotation to the test to label it as a web test that can be switched off if required, e.g.,
@skipIf(os.environ.get('XARRAY_EOPF_DISABLE_WEB_TESTS', '1') == '1',
'XARRAY_EOPF_DISABLE_WEB_TESTS = 1')
There was a problem hiding this comment.
Unit tests are located in tests/ and run on every push as well as when running pytest. Integration tests access real data through the EOPF Sample Service infrastructure. They are located in integration/ and run only when executing `pytest integration/ or via a daily GitHub Actions cron job.
| if grd is None: | ||
| grd = datatree[group].measurements.to_dataset() | ||
| grd = grd.rename({"grd": mode.lower()}) | ||
| assert len(children) == 1 |
There was a problem hiding this comment.
Why is it important there is exactly one child?
There was a problem hiding this comment.
For each polarization mode there should be only one child, otherwise the product is invalid. I will logging to it, otherwise user will not understand.
Probably you have not put in CDSE credentials which is needed to retrieve the COP DEM 30m from CDSE STAC API. I will give them to you for testing. Otherwise, I have corrected some typos and small things which I noticed on the way. |
TonioF
left a comment
TonioF
left a comment
There was a problem hiding this comment.
Thanks, the notebook made it a lot clearer to understand the implementation. I approve now.
2 participants
closes #67